Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable serf encryption #1791

Merged
merged 10 commits into from
Oct 17, 2016
Merged

Enable serf encryption #1791

merged 10 commits into from
Oct 17, 2016

Conversation

diptanu
Copy link
Contributor

@diptanu diptanu commented Oct 4, 2016

@dadgar @slackpad for review.

@Gerrrr Please help us test this feature if you can!

@diptanu diptanu mentioned this pull request Oct 5, 2016
Copy link
Contributor

@dadgar dadgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left small comments + tests failures should be fixed

@@ -68,6 +68,7 @@ server {
retry_max = 3
retry_interval = "15s"
rejoin_after_leave = true
encrypt = "abc"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

// KeyringResponse is a unified key response and can be used for install,
// remove, use, as well as listing key queries.
type KeyringResponse struct {
Messages map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on these?

helpText := `
Usage: nomad keyring [options]

Manages encryption keys used for gossip messages between nomad servers. Gossip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nomad should be capital

}
var resp KeyringResponse
_, err := a.client.write("/v1/agent/keyring/install", &args, &resp, nil)
return &resp, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above you do the extra if err != nil { return nil, err } check, so maybe keep all of these methods consistent in what they return?

}

// KeyringRequest is request objects for serf key operations.
type KeyringRequest struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused? Looks like you use structs.KeyringRequest instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using them now.

@@ -68,6 +68,7 @@ server {
retry_max = 3
retry_interval = "15s"
rejoin_after_leave = true
encrypt = "abc"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

}
defer fh.Close()

if _, err := fh.Write(keyringBytes); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just use ioutil.WriteFile instead of OpenFile+Write, but you'd still need the Remove on error.

@@ -16,6 +18,19 @@ type Agent struct {
region string
}

// KeyringResponse is a unified key response and can be used for install,
// remove, use, as well as listing key queries.
type KeyringResponse struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just use the struct package's KeyringResponse instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schmichael The API package mimics the structs package largely but gives a separation between internal and external structs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that explains it! Thanks. (Wish I could mark comments as addressed in this new PR mode without full on deleting them...)

@@ -391,6 +391,16 @@ configured on client nodes.
join any nodes when it starts up. Addresses can be given as an IP, a domain
name, or an IP:Port pair. If the port isn't specified the default Serf port,
4648, is used. DNS names may also be used.
* <a id="encrypt">`encrypt`</a> Specifies the secret key to use for encryption
of Nomad server's gossip network traffic. This key must be 16-bytes that are
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or 24 or 32? Or do we not even want to bother mentioning the aes 192 & 256 support?

# Command: `keyring`

The `keyring` command is used to examine and modify the encryption keys used in
Nomad server. It is capable of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reformat lines

t.Fatalf("bad: %v", kresp)
}
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to also add a test for use operation?

@Gerrrr
Copy link
Contributor

Gerrrr commented Oct 6, 2016

@diptanu Looks great!

Does it make sense to add general options to the keyring command? I was not able to use it when the server is not listening on local interface:

$ bin/nomad server-members -address=http://172.28.128.3:4646
Name                    Address       Port  Status  Leader  Protocol  Build     Datacenter  Region
nomad-server01.region1  172.28.128.3  4648  alive   false   2         0.5.0dev  dc1         region1
nomad-server02.region1  172.28.128.4  4648  alive   true    2         0.5.0dev  dc1         region1
nomad-server03.region1  172.28.128.5  4648  alive   false   2         0.5.0dev  dc1         region1

$ bin/nomad keyring -list
==> Gathering installed encryption keys...
error: Get http://127.0.0.1:4646/v1/agent/keyring/list: dial tcp 127.0.0.1:4646: getsockopt: connection refused

$ bin/nomad keyring -list -address=http://172.28.128.3:4646
flag provided but not defined: -address

@diptanu
Copy link
Contributor Author

diptanu commented Oct 8, 2016

@Gerrrr Definitely, good catch! I can do that, only have internet access via cybercafes at the moment as I am on vacation, but consider it done by the time we make the final 0.5 release.

@diptanu diptanu merged commit f0806dc into master Oct 17, 2016
@diptanu diptanu deleted the f-serf-encrypt branch October 17, 2016 17:48
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants